Skip to content

refactor(speculative): one home each for the DFlash family's shared rules - #2757

Merged
h-guo18 merged 15 commits into
mainfrom
haoguo/dflash-family-cleanup
Oct 30, 2026
Merged

h-guo18 merged 15 commits into
mainfrom
haoguo/dflash-family-cleanup

Conversation

@h-guo18

@h-guo18 h-guo18 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Type of change: Refactor, no behavior change

Rules the DFlash family wrote out more than once now live in one place each:

  • Block supervision. Which token each block position is trained on, which positions count, and how they are weighted were written out five times (DFlash, DFlash2's selector, LiLiCorr, DSpark, Domino), plus three hand-written teacher-forced token gathers. They are now _block_targets, _block_tokens and _block_position_weights, driven by one class constant, _label_shift: DFlash, DFlash2 and LiLiCorr train position k on the token at anchor + k, and DSpark and Domino on anchor + k + 1. _first_predicted_position(shift) derives the first supervised position from it, for the mask and the weighting alike. DSpark and Domino still always use decay weighting, as today: they never read dflash_loss_objective. A follow-up makes them honor it.
  • Teacher shift. The base logits that predict the token at position i sit at i - 1. DFlash KD, DSpark and LiLiCorr each applied that shift before calling _teacher_logits. It now takes label positions and shifts them itself, and every caller passes the positions it supervises. A wrong shift trains silently, just worse. label_positions is keyword-only, so a caller still passing logit rows positionally raises instead of reading one row early.
  • Final norm. _apply_base_final_norm(hidden, prenorm, norm) takes the flag as a bool. _maybe_apply_base_final_norm keeps reading it from the producer's dict for EAGLE, and delegates.
  • Zero loss. A step with nothing to supervise returns a zero loss that every trainable draft parameter feeds, so DDP still reduces each one. Six copies in four forms become _draft_zero_loss, which also says which route to take. Before the draft has run (forward()'s early returns), it reads one element of each parameter, not a full sum() of every weight. Once it has run, the caller passes the draft's outputs as through and the loss goes through them: with DFlashModule as its own FSDP2 unit (the MiniMax-M2.7 launcher layout), reading parameters FSDP2 has resharded puts that rank's collectives out of step and aborts the job. DSpark passes its final and confidence logits, since the confidence head is not behind the final logits.
  • Accuracy. Five copies of the masked top-1 accuracy become _masked_top1.
  • Target layers. hidden_states[lid + 1] for each target layer, in three forwards and both generation paths, becomes _target_hidden.
  • Generation. DFlash and DSpark's pseudo_speculative_generate shared their first half; it is now _generation_base + _draft_block. The block's position ids come from the training _build_position_ids at anchor = ctx_len.
  • Generation mask. _build_generate_swa_mask hand-wrote the training visibility rule a second time. It now evaluates _draft_mask_mod for its one block, anchored at ctx_len, through the dense helper the training mask also uses.
  • Config defaults. Thirteen getattr(config, name, <default>) reads, covering fifteen variant fields, duplicated DFlashConfig's own defaults. One pair had drifted: dflash_fp32_master_weights defaults to True in the config and False in the fallback. They are now read directly. projector_type dispatch goes through a table instead of an if/elif chain.

Net change to source against main: +389 / −430 lines.

Usage

No API change.

Testing

  • tests/unit/torch/speculative (+ tests/unit/torch/export/test_hf_spec_rope_export.py): 505 passed on transformers 5.5.4, 5.12.1 and 5.18.0 (5.5.4 and 5.18.0 fall within CI's tf_min and tf_latest pins). tests/gpu/torch/speculative/plugins: 17 passed, 1 skipped (the sharded-generation test needs 2 GPUs).
  • Bitwise A/B against main and against the previous head of this PR. One training step for each of 23 cases, plus one generation step for the 7 online ones, in fp32 and bf16 (46/46 identical against each): loss, every gradient, the variants' metrics, and the generated draft tokens.
    • DFlash: KD and CE.
    • DFlash2: ce / tv / lambda objectives, with the selector.
    • LiLiCorr: with the penalty and calibration terms.
    • DSpark: vanilla and rnn heads, confidence head on.
    • Domino.
    • Together the cases run offline and online, with D-PACE and decay weighting, including steps that take the zero-loss branch.
  • FSDP2 (2 ranks, gloo, base decoder layers and the draft each their own fully_shard unit, one rank on the in-loss zero branch while the other trains):
    • DFlash, CE and KD: both ranks finish, as before.
    • DSpark, with compute_markov_bias and compute_confidence_logits registered through register_fsdp_forward_method: going through the final and confidence logits finishes. Every form that reads parameters fails: parameters only, final output plus one element per parameter, and main's final output plus p.sum(). Going through the final output alone fails too, since the confidence head then has no gradient.
    • Without that registration, no DSpark step runs in this layout, on main either, because the heads run outside the draft's forward. Domino's GRU head has the same limitation. With the draft inside the root unit, DSpark and Domino both finish.
  • After the review, for the seven commits on top: the five DFlash-family unit test files (test_hf_{dflash,dflash2,lilicorr,dspark,domino}.py, 308 tests) pass on transformers 5.5.4, 5.12.1 and 5.18.0 (the last two commits, which only add argument checks and keyword-only arguments, on 5.12.1), and tests/gpu/torch/speculative/plugins/test_hf_dflash.py passes on 2 GPUs (15 passed). The 23-case A/B of the first two pushes against the head before each is bitwise identical (46/46); the third push was not A/B'd. The full speculative suite was not re-run.
  • New tests:
    • Label 0 reads row 0 rather than wrapping to the last row.
    • The generation mask equals the old hand-written one (kept verbatim in the test) over 18 combinations of context length, window and block causality.
    • Each variant's labels and supervision on a hand-worked example. This one fails if DSpark's shift is 0 or the anchor slot stops being excluded; no existing test caught either.
    • DSpark's zero-weight branch gives its final and confidence logits a gradient, the route an FSDP2 unit needs. This one fails against the previous head and against every parameter-reading or final-only form above.
    • DFlash's and Domino's zero-loss steps, with no block kept and with a kept block that has no weighted label, give every trainable draft parameter a gradient and, once the draft has run, its output too. Switching either in-loss zero to parameter reads fails it.

Before your PR is "Ready for review"

  • Is this change backward compatible?: ✅
  • If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
  • Did you write any new necessary tests?: ✅
  • Did you update Changelog?: N/A
  • Did you get Claude approval on this PR?: N/A

Additional Information

Reuse. The shared rules are module-level helpers next to HFDFlashModel, which every variant already subclasses. projector_type dispatch keeps the existing per-variant registries. The variants differ only in one class constant, _label_shift, plus the loss terms each keeps in its own subclass, so no new policy or config object is needed. The one existing helper close to _masked_top1 is HFExternalDraftModel._top1_accuracy. It divides by clamp_min(1) instead of adding 1e-6, and tests mask.bool() instead of mask > 0.5. Sharing it would change the DFlash family's logged accuracy, which this refactor keeps bitwise identical.

Follow-up to the design review of #2279.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Improvements
    • Speculative decoding models use more consistent token targeting, attention masking, and accuracy calculations across training and generation.
  • Bug Fixes
    • Teacher-logit selection handles label position zero without wrapping to the final position.
    • Unsupported projector types now produce an error listing supported options and identifying the default.

@copy-pr-bot

copy-pr-bot Bot commented Oct 12, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between e13d618 and a5d6a6b.

Files selected for processing: 3
  • modelopt/torch/speculative/plugins/hf_dflash.py
  • modelopt/torch/speculative/plugins/hf_domino.py
  • modelopt/torch/speculative/plugins/hf_dspark.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The pull request adds shared DFlash-family helpers for target construction, loss calculation, normalization, and generation. It updates DFlash, DFlash2, Domino, DSpark, and LiLiCorr to use these helpers. Conversion selects registries by projector type.

Changes: DFlash-family model behavior

Layer / File(s) Summary
Shared targets and model helpers
modelopt/torch/speculative/plugins/hf_dflash.py, modelopt/torch/speculative/plugins/modeling_final_norm.py
Shared helpers define block positions, targets, tokens, weights, accuracy, hidden-state selection, zero loss, teacher-logit indexing, and dense masks. Final-norm application accepts an explicit pre-norm flag.
Shared loss integration
modelopt/torch/speculative/plugins/hf_dflash.py, modelopt/torch/speculative/plugins/hf_dflash2.py, modelopt/torch/speculative/plugins/hf_domino.py, modelopt/torch/speculative/plugins/hf_dspark.py, modelopt/torch/speculative/plugins/hf_lilicorr.py, tests/unit/torch/speculative/plugins/test_hf_dspark.py, plugins/modelopt/skills/speculative-decoding/references/algorithms/dspark.md
The five model implementations use shared target and loss helpers. Several configuration fields now require explicit attributes. The DSpark reference broadens its offline-model error pattern.
Generation masks and draft execution
modelopt/torch/speculative/plugins/hf_dflash.py, modelopt/torch/speculative/plugins/hf_dspark.py, tests/unit/torch/speculative/plugins/test_hf_dflash.py, plugins/modelopt/skills/speculative-decoding/references/algorithms/dflash.md
DFlash generation uses the shared visibility rule and shared base-model and draft-block helpers. DSpark uses inherited generation helpers. Tests compare generation masks with a reference implementation.
Target and gradient validation
tests/unit/torch/speculative/plugins/test_hf_dflash.py, tests/unit/torch/speculative/plugins/test_hf_dspark.py
Tests cover target shifts, supervision masks, teacher-logit indexing, generation masks, and zero-loss gradient coverage.

Projector registry selection

Layer / File(s) Summary
Projector-to-registry routing
modelopt/torch/speculative/dflash/conversion.py
Conversion selects a registry from a projector-type mapping. Unsupported values produce an error that lists supported types and identifies the default.

Priority: Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant DFlashGeneration
  participant BaseModel
  participant DraftBlock
  participant DraftModule
  DFlashGeneration->>BaseModel: _generation_base runs the base model
  DFlashGeneration->>DraftBlock: _draft_block builds the anchored block
  DraftBlock->>DraftModule: Runs the draft module with embeddings and attention mask
Loading

Possibly related issues

Possibly related PRs

Suggested reviewers: kevalmorabia97, meenchen, shengliangxu

Merge Risk: Minimal · previous review a5d6a

No actionable merge-blocking risk was identified.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: centralizing shared rules across the DFlash family through a refactor.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Security Anti-Patterns Passed The pull request changes only DFlash-family Python modules, tests, and documentation. The added lines contain no torch.load(..., weights_only=False), numpy.load(..., allow_pickle=True), hardcoded …
Full details: Security Anti-Patterns

Explanation

The pull request changes only DFlash-family Python modules, tests, and documentation. The added lines contain no torch.load(..., weights_only=False), numpy.load(..., allow_pickle=True), hardcoded trust_remote_code=True, external-input eval()/exec(), or # nosec. It also adds no pyproject.toml or requirements.txt dependency changes.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.96%. Comparing base (0638a27) to head (112123e).
⚠️ Report is 90 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2757      +/-   ##
==========================================
- Coverage   79.82%   76.96%   -2.87%     
==========================================
  Files         661      669       +8     
  Lines       74928    87917   +12989     
==========================================
+ Hits        59810    67662    +7852     
- Misses      15118    20255    +5137     
Flag Coverage Δ
examples-diffusers 19.95% <18.47%> (+0.01%) ⬆️
examples-gpt-oss 13.49% <18.47%> (+0.01%) ⬆️
examples-hf_ptq 23.39% <18.47%> (-0.03%) ⬇️
examples-llm_distill 13.56% <18.47%> (+0.01%) ⬆️
examples-llm_eval 17.21% <18.47%> (+0.01%) ⬆️
examples-llm_qat 17.38% <18.47%> (-0.02%) ⬇️
examples-llm_sparsity 16.80% <18.47%> (+0.01%) ⬆️
examples-megatron_bridge 26.45% <18.47%> (-0.20%) ⬇️
examples-specdec_bench 13.27% <18.47%> (+0.01%) ⬆️
examples-speculative_decoding 17.47% <20.38%> (-0.05%) ⬇️
examples-torch_onnx 21.26% <18.47%> (-0.02%) ⬇️
examples-torch_trt 15.19% <18.47%> (+0.01%) ⬆️
examples-vllm_serve 17.43% <18.47%> (+0.01%) ⬆️
gpu 59.00% <49.04%> (-1.08%) ⬇️
regression 15.08% <59.23%> (+<0.01%) ⬆️
unit 62.43% <100.00%> (+1.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-30 16:56 UTC

Base automatically changed from haoguo/dflash-training-perf to main October 23, 2026 07:36
h-guo18 added a commit that referenced this pull request Oct 23, 2026
### What does this PR do?

Type of change: new feature (performance), bug fix

Speeds up DFlash-family drafter training. On Gemma-4-E4B DSpark
streaming (PDX B300, 3 nodes = 1 vLLM serve + 2 trainers, 16 DP ranks,
batch 4 × 4096), step time goes **1.690 → 0.484 s (3.49×)**, with the
loss unchanged within run-to-run noise. Every round below was measured
on the same window (steps 21-40, profiler off).

| Change | Step time |
|---|---|
| FlexAttention over a block-sparse draft mask
(`dflash_use_flex_attention`, opt-in). The dense `[B, 1, Q, KV]` mask
had forced SDPA onto its sm80-only memory-efficient kernel. | 1.690 →
0.711 s |
| Offline/streaming and DSpark online: project only the base-teacher
rows a loss reads, not full `[B, seq, vocab]` logits; one device sync
for DSpark's metrics instead of five | 0.711 → 0.629 s |
| DSpark TVD chunked with `Tensor.split` instead of slicing, whose
backward zero-filled a full `[N, vocab]` tensor per chunk | 0.629 →
0.541 s |
| `torch.compile` of the draft stack and the DSpark TVD chunk
(`dflash_use_torch_compile`, opt-in). With compile or FlexAttention on,
the anchor count is fixed by the config and `seq_len`, so the compiled
shapes stay static: recompiles in 40 steps drop from 4 to 1. Eager runs
still size it to the batch. | 0.541 → 0.484 s |

The same rounds, by phase and by GPU kernel:

<img width="2040" height="2040" alt="image"
src="https://github.com/user-attachments/assets/13ae02e5-a171-4f48-8739-c6828d018413"
/>

Also:
- Offline/streaming LiLiCorr (#2589) reads its candidate target logits
the same per-row way, instead of rebuilding full-sequence logits.
- FlexAttention's conflicts with the attention sink and with dropout are
rejected at convert time.
- No-behavior-change cleanups: the attention visibility rule is written
once (`_draft_mask_mod`) for SDPA and FlexAttention, and KD is switched
by `dflash_self_logit_distillation` rather than by a nullable argument.

**Why not HF's `flex_attention` dispatch (the path EAGLE takes)?** The
BlockMask is built here either way; the difference is the kernel call.
HF's `flex_attention_forward` passes `enable_gqa=True` whenever the
query-head count is a power of two, and that backward took 568 ms on the
Gemma-4-E4B drafter: 10× the path that repeats K/V first, and 2.5×
slower than the SDPA baseline. At `head_dim` > 256, FlexAttention's
default tiles fail on sm103 (misaligned address, launch failure), so the
kernel tiles are pinned to 32×32 and the BlockMask block size has to
divide them; `dflash_flex_attention.py` keeps both rules together.

### Usage

```yaml
dflash:
  dflash_use_flex_attention: true
  dflash_use_torch_compile: true
```

On multi-node runs, point `TRITON_CACHE_DIR` and
`TORCHINDUCTOR_CACHE_DIR` at node-local paths: on a shared filesystem,
concurrent ranks race on the compile cache.

### Testing

- `tests/unit/torch/speculative`: 387 passed on transformers 5.12.
`tests/gpu/torch/speculative/plugins`: 14 passed, 1 skipped.
- FlexAttention on torch 2.8.0 (manual, RTX PRO 6000): against an fp64
dense reference on padded-row shapes (seq 128, Q_LEN 144 and 244),
forward and backward agree to ≤ 7e-7 relative in fp32, and
compute-sanitizer memcheck is clean. Before the `q_block` clamp,
memcheck caught the backward reading past `anchors`.
- New tests cover FlexAttention vs. dense (GPU), the mask rule against a
hand-written mask and on padded FlexAttention rows, TVD chunk-size
invariance and `split` in the backward graph, anchor sampling vs. the
legacy sampler in eager and static modes, the per-row teacher (lm_head
never sees the full sequence offline), and FlexAttention conflicts at
convert time and through a `from_pretrained` resume.
- The cleanups were checked old vs. new: losses, gradients, dense masks
and GPU BlockMasks are bitwise equal, except DSpark's rnn head, which
differs at bf16 rounding.
- Against main, offline DFlash KD, LiLiCorr and DSpark get bitwise-equal
losses and gradients in bf16 from the per-row teacher. The DFlash
regression recipe (eager) trains in the same time as on main with an
identical loss curve.
- Step times are medians over 16 ranks × steps 21-40 (profiler off) on
the setup above.

### Before your PR is "*Ready for review*"

- Is this change backward compatible?: ✅ Both speedups are opt-in:
`dflash_use_flex_attention` and `dflash_use_torch_compile` default off.
The latter defaulted to True on main but did nothing; the shipped
recipes set it false.
- If you copied code from any other sources or added a new PIP
dependency, did you follow guidance in `CONTRIBUTING.md`: N/A
- Did you write any new necessary tests?: ✅
- Did you update
[Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?:
❌
- Did you get Claude approval on this PR?: ❌

### Additional Information

Based on main. The step times were measured on Gemma-4-E4B, which needs
#2186 to run; nothing in this PR is Gemma-specific.

The streaming `/done` fix that was part of this PR is now #2756.
Follow-up refactors are in draft #2757, stacked on this PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added an optional FlexAttention path for DFlash draft attention, with
block-sparse masking.
* Added optional training-time compilation for draft model components;
compilation is disabled by default.
* Teacher logits can now be obtained from supplied logits or captured
hidden states during distillation.
* **Bug Fixes**
* Improved draft training behavior for batches with no valid anchors,
ensuring trainable parameters receive gradients.
* Added validation for FlexAttention configurations that do not support
attention sinks or dropout.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Hao Guo <haoguo@nvidia.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@h-guo18
h-guo18 force-pushed the haoguo/dflash-family-cleanup branch from 28eb032 to 9377aa7 Compare October 23, 2026 07:36
h-guo18 and others added 7 commits October 24, 2026 09:59
…norm core

The next-token shift -- the base logits that predict the token at position i sit at i - 1 --
was spelled out at each of _teacher_logits' three callers: DFlash KD and DSpark as
(safe_label_indices - 1).clamp(min=0), LiLiCorr as anchor + s for the token at anchor + 1 + s.
A wrong shift trains without an error, just worse, so it now lives once: _teacher_logits
takes the label positions and applies the shift itself, and the callers pass what they
supervise. DFlash2's predecessor gather uses the same offset for a different quantity (the
previous token, not the teacher) and keeps its own.

_maybe_apply_base_final_norm took the producer's whole outputs dict, so _teacher_logits
wrapped its one flag in a single-key dict to call it. The bool now has its own function,
_apply_base_final_norm, which the dict reader delegates to; EAGLE keeps calling the dict
reader, unchanged.

Behavior is unchanged: against this branch's parent, offline DFlash KD, LiLiCorr (penalty and
calibration) and DSpark give bitwise-equal losses and gradients in fp32 and bf16, with pre-
and post-norm captures stored in float64. The teacher tests now pass labels and expect row
label - 1, and fail without the shift; a new test pins that label 0 reads row 0 rather than
wrapping to the last row.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
…he DFlash family

Two pieces of the family's forwards were written out once per model.

- The step with nothing to supervise returns a zero loss that every trainable draft parameter
  feeds, so DDP's reduction still sees each of them. There were four copies in two forms:
  DFlash's indexed one element of each trainable parameter, while Domino's forward, DSpark's
  forward and DSpark's degenerate-loss branch summed every whole parameter, reading all draft
  weights to produce a zero. HFDFlashModel._draft_zero_loss now has the cheap form once.
- The concatenation of the base layers the draft's fc reads, hidden_states[lid + 1] for each
  target layer, appeared in the DFlash, DSpark and Domino forwards. It is now
  HFDFlashModel._target_hidden, which also carries the "+ 1" (hidden_states[0] is the
  embedding output) that modify()'s check on negative ids refers to.

Losses and gradients are unchanged; DSpark's DDP-coverage tests, which require a gradient on
every draft parameter for a batch with no valid anchor and for one with zero total weight,
pass on the shared helper.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
_build_generate_swa_mask hand-wrote the draft's visibility rule a second time -- context
before the block, the window counted from each query's real position, block-causal under
dflash_draft_attention="causal" -- and its docstring asked readers to keep it in step with
training. It is the training rule, _draft_mask_mod, for the one block being drafted,
anchored at ctx_len, so it now evaluates that. The grid evaluation and the additive
conversion that the training mask's dense path already did become _dense_additive_mask,
shared by both.

A verbatim copy of the old builder stays in the tests as the reference: over contexts of 1,
5 and 16 tokens, no window, a window of one block and of two, and bidirectional and causal
blocks, the masks are equal element for element (and None where the old one returned None).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
Which token each block position is trained on, which positions count, and how they are
weighted were written out five times: DFlash _compute_loss, DFlash2 _compute_loss (for the
selector), LiLiCorr _block_targets, DSpark _compute_dspark_loss and Domino
_compute_domino_loss. The copies differed only in the label offset (DFlash, DFlash2 and
LiLiCorr train position k on the token at anchor + k; DSpark and Domino on anchor + k + 1)
and in whether slot 0 is skipped, so a fix to one never reached the others.

- _block_targets returns the label token, its sequence position and the supervision mask,
  given the class constant _label_shift (0, or 1 for DSpark and Domino). The teacher reads
  its rows from those positions, including LiLiCorr's candidate logits.
- _block_tokens is the teacher-forced token gather that DSpark's and Domino's heads and
  DFlash2's predecessor ids each wrote by hand.
- _block_position_weights holds the D-PACE and decay weighting. DSpark and Domino pass
  "decay" explicitly, which is what they do today: they never read dflash_loss_objective.
- _masked_top1 replaces five copies of the masked top-1 accuracy.

The in-loss zero branches keep routing through the draft's output (flat_logits.sum() * 0.0)
rather than _draft_zero_loss: with DFlashModule as its own FSDP2 unit (the MiniMax-M2.7
launcher layout), reading the resharded parameters directly skips the unit's backward
collectives on that rank and the job aborts with a collective mismatch.

Behavior is unchanged. Against the previous head, a train step of DFlash (KD and CE),
DFlash2 (ce, tv and lambda, with the selector), LiLiCorr, DSpark (vanilla and rnn heads,
confidence head on) and Domino, offline and online, with D-PACE and decay, and steps that
take the zero-loss branch, gives bitwise-equal loss, gradients and metrics in fp32 and bf16.
DFlash2's predecessor ids now come from the shared gather; they differ from the old
formula only at positions whose label lies past the sequence end, which carry no selector
weight.

A new test pins each variant's labels and supervision on a hand-worked example; it fails if
DSpark's shift is set to 0 or if the anchor slot stops being excluded. No existing test did.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
…Spark

DFlash's and DSpark's pseudo_speculative_generate repeated the same first half: run the
base model on the prefix, take its greedy next token, gather the target layers onto the
draft's device, build the block (anchor then mask tokens), its position ids and mask, and
run the draft. Only the decoding after that differs (DFlash takes the argmax of positions
1..bs-1; DSpark walks its Markov head over the block).

- _generation_base returns the base model's next token and hidden states, and carries the
  offline check both copies made.
- _draft_block drafts the block that token anchors. Its position ids now come from
  _build_position_ids at anchor = ctx_len -- the training layout -- the way the generation
  mask already comes from the training mask rule. The comment it replaces described a
  uniform-block layout training has not used since the anchors were introduced.
- _target_hidden takes an optional device, so the last two hand-written
  hidden_states[lid + 1] picks go through it.

The docstring's note that generation runs with attention_mask=None was stale since the
generation mask was added and now says what is passed.

Generated tokens are unchanged: DFlash (full attention, sliding window, causal blocks),
DFlash2, LiLiCorr, DSpark and Domino online give bitwise-equal draft tokens in fp32 and bf16
against the previous commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
… a second default

Thirteen variant fields were read as getattr(config, "dflash_...", <default>) although
DFlashConfig declares every one of them with a default, so each default lived twice. One
pair had already drifted: dflash_fp32_master_weights defaults to True in the config and to
False in HFDFlashModel.modify's fallback, which a reader would take for the default. The
fallbacks never run, because modify() always receives a DFlashConfig, so the fields are now
read directly.

convert_to_dflash_model chose the registry for dflash_architecture_config.projector_type
with an if/elif chain and listed the accepted names again by hand in its error. A table
maps each projector type to its registry, and the error lists the table's keys.

No behavior change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
…e skill tables

The shared generation step raises one message for the whole family, "An offline
DFlash-family model cannot run AR validation / pseudo_speculative_generate". The
known-failure rows in the DFlash and DSpark skill references still quoted the two
per-variant messages it replaced, so a search for the actual error found no row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
@h-guo18
h-guo18 force-pushed the haoguo/dflash-family-cleanup branch from 9377aa7 to 025e18a Compare October 24, 2026 13:47
h-guo18 added a commit that referenced this pull request Oct 24, 2026
…evice

DSpark's pseudo_speculative_generate adds its Markov bias, computed on
the draft's device, to the backbone logits that the base model's lm_head
returns. scripts/ar_validate.py loads with device_map="auto", and
accelerate keeps a tied lm_head with embed_tokens on the first GPU while
the draft can land on a later one. The logits then stay on the first
GPU, so the sum raises "Expected all tensors to be on the same device"
for every sample and AR validation reports nothing. This has been the
case since DSpark was added.

The backbone logits now move to the draft's device before the loop. On
a single device the move is a no-op.

Verified: a new GPU test dispatches the model with accelerate's
dispatch_model, embed_tokens and lm_head on the CPU and everything else
on cuda:0, and generates with DFlash and DSpark. DSpark fails without
the fix and passes with it. The bitwise A/B against #2757 shows DSpark's
drafted tokens unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
h-guo18 added a commit that referenced this pull request Oct 24, 2026
…nt and head

HFDominoModel did not override pseudo_speculative_generate, so AR
validation ran DFlash's, which decodes block positions 1..bs-1 with the
backbone alone. DFlash trains position k on the token at anchor + k;
Domino trains it on anchor + k + 1 (_label_shift = 1). Every drafted
token was therefore compared with the target one position too early,
so drafts were almost never accepted, and the correction head that
training optimizes and the exporter ships was never applied. domino.yaml
keeps AR validation off, so shipped training did not hit this.

DFlash's default decode now starts at block position 1 - _label_shift
and returns at most bs - 1 + _label_shift tokens. For DFlash, DFlash2
and LiLiCorr (shift 0) that is the same slice as before. Domino
overrides it the way DSpark does for its Markov head: it decodes the
block left to right, stepping the GRU over the anchor and the tokens
drafted so far, and from pure_draft_prefix_len on adds
embed_proj([hidden_k; GRU state]) to the backbone logits, as
_apply_domino_head does in training. As in DSpark's decode, the
backbone logits and the drafted tokens' embeddings are moved to the
draft's device: under device_map="auto" the embedding table, and a tied
lm_head with it, can sit on another GPU.

The eval forward warned that Domino's acceptance rates were
backbone-only, but that method computes no acceptance rate, and AR
validation now runs the head. The warning goes, and so do the other
copies of the claim: the comments in domino.yaml, dspark.yaml and the
Nemotron-3.5 DSpark warm-start recipe, the note in the Domino launcher
example, and the speculative-decoding skill's Domino, DSpark and
validation sheets. DSpark's generation has applied its Markov head
since DSpark was added. The validation sheet keeps its rule against
reporting such an AR as the model's result, now for the heads that AR
validation still skips (LiLiCorr's reranker, DFlash2's selector), and
adds the LiLiCorr and online DFlash2 examples to the ones it lists as
shipping no benchmark task. The Domino sheet and example now say how to
add the HF AR eval: DFlash's task_2 with --steps equal to the block
size, since Domino drafts the whole block. The flag values do not
change.

Verified: a new test checks that Domino's greedy drafts equal the
argmax of _apply_domino_head teacher-forced on them, with head weights
large enough to overturn the backbone. It fails on the parent commit,
and also when the head is skipped, its GRU state lags a step, or the
prefix boundary is off by one. The GPU test that puts lm_head on
another device now covers Domino as well, and fails without the move.
Bitwise A/B against #2757: DFlash and DSpark loss, gradients and drafted
tokens are unchanged; only Domino's drafted tokens change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
@h-guo18
h-guo18 marked this pull request as ready for review October 27, 2026 13:50
@h-guo18
h-guo18 requested review from a team as code owners October 27, 2026 13:50
@h-guo18
h-guo18 requested a review from ChenhanYu October 27, 2026 13:50
h-guo18 and others added 3 commits October 27, 2026 15:08
Which block position is the first to predict a token after the anchor was written out in two
forms: `k + shift > 0` in _block_targets and `1 - self._label_shift` at each of the three
_block_position_weights calls. _first_predicted_position(shift) now holds it. Its
`max(0, 1 - shift)` also stays right for a shift above 1, where `1 - shift` would turn
negative and the weights' `[..., first:]` slices would quietly start from the block's end.

No behavior change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
…draft parameter

Only DSpark's zero losses had a unit test. DFlash's in-loss zero, Domino's in-loss zero and
Domino's early return ran in none, and codecov reported those three lines uncovered.

The new test runs a forward with no anchorable answer, and one whose single kept block has
no label in the mask. It requires a gradient on every trainable draft parameter. Once the
draft has run, it also requires one on the draft's output, the route an FSDP2 unit around
the draft needs. Switching either in-loss zero to parameter reads fails it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
…ontext length

ctx_len only feeds _build_generate_swa_mask, so parametrizing over it repeated the same
tiny-model conversion for each of three context lengths. The test now converts six models
instead of eighteen and still compares the mask at the same 18 points. A failure names the
context length.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
@h-guo18

h-guo18 commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Claude review — refactor(speculative): one home each for the DFlash family's shared rules

Scope: full review (the trigger comment carried no scoping instructions). 11 changed files, all opened: 7 under modelopt/torch/speculative/, 2 unit tests, 2 skill-reference docs. No examples/ changes.

Findings: CRITICAL: 0 · IMPORTANT: 0 · SUGGESTION: 2

Previous round's blocker is resolved

The one IMPORTANT from the last pass — DSpark's degenerate-loss branch routing only through parameters — is fixed at this head. hf_dspark.py:233 now passes through=(flat_final, confidence_logits), and I traced that the pair genuinely covers every trainable draft parameter for all three head types: markov_w1/markov_w2 and the type-specific gate_proj / joint_proj are all consumed by compute_markov_bias (modeling_dspark.py:163-190) and so sit behind final_logits; the backbone is reached through base4d = lm_head(hidden); confidence_proj is the only parameter outside that cone, and the second element covers it. test_hf_dspark.py's added retain_grad assertions pin exactly that route. Last round's two SUGGESTIONs are half-addressed: first is now derived by _first_predicted_position and the ctx_len parametrization became an in-test loop, while the target_ce=None trap remains.

Non-blocking

  • [SUGGESTION] hf_dflash.py:882-896 — _draft_zero_loss's opening docstring line promises every trainable draft parameter, which only the parameter route delivers; the through route reaches only what is behind the tensors passed. Since the gap it hides is the bug the old DSpark branch carried, the summary line is worth tightening. Plus: through=(None,) returns the int 0 rather than a tensor.
  • [SUGGESTION] hf_dflash.py:185 — _block_position_weights takes decay_factor and dpace_alpha as adjacent same-typed positional floats, passed positionally at all three call sites; keyword-only would apply the same rule the PR applies to label_positions. target_ce is still called unconditionally in the D-PACE branch despite defaulting to None.

What I verified as equivalent

Sold as a no-behavior-change refactor, so each extraction was traced against the code it replaces rather than read in isolation. New this round, beyond last pass's list:

  • _block_targets ordering. All three sites take binary_eval_mask from targets.mask before calling _block_position_weights, and that function returns new tensors (mask * dpace, mask * decay) without mutating its input — so the eval mask stays the pre-weighting one, as on main.
  • DSpark/Domino decay. _first_predicted_position(1) == 0 makes exp(-(k - 0).clamp(min=0)/gamma) the old exp(-k/gamma); DFlash's shift=0 gives first=1 and recovers (k-1). The old if dpace / elif decay and the new if dpace: return / if decay agree, block_size == 1 fall-through included.
  • _masked_top1 dtype. correct.sum().float() / (mask.sum() + 1e-6) keeps the result float32, so DSpark's five-scalar torch.stack still sees one dtype under a bf16 draft.
  • LiLiCorr candidate labels. The new double-clamp differs from the old single clamp only where anchor+1+s >= seq_len; both the penalty (hf_lilicorr.py:502) and calibration (:557) terms multiply by supervised = forced_prefix * valid_block with valid_block = slot_mask.all(dim=-1), so any block containing such a slot is zeroed whole and the differing teacher row never enters either loss.
  • Generation mask. At anchor = ctx_len, n_blocks = 1: q_block collapses to 0, ctx_ok to kv_idx < ctx_len, the window clause to kv_idx > ctx_len + q_idx - window, the causal clause to (kv_idx - ctx_len) <= q_idx, and keep[b, 0] to True — term for term the hand-written rule. dflash_attention_sink is applied inside the draft attention (modeling_dflash.py:265), not in the mask, so nothing silently joins the rule.
  • _build_position_ids(ctx_len, [[ctx_len]], device) yields [0..ctx_len-1, ctx_len..ctx_len+bs-1], identical to both torch.arange concatenations it replaces.
  • Config reads. All 15 fields are declared on DFlashConfig (config.py:244-442) and restore reconstructs it through Pydantic, so every removed fallback was dead on every path, LiLiCorr's -1.0 unset sentinels included. The dflash_fp32_master_weights drift was real; the config's True was already what every caller got.
  • Registry table. None is a key mapping to DFlashDMRegistry, so registry is None fires only on genuinely unknown types.
  • _maybe_apply_base_final_norm survives for hf_eagle.py:713, hf_external.py:121 and its own test file; hf_dflash.py:924 reads the same attribute the old dict wrapper read.
  • Dtype/overflow. Swapping sum(p.sum() for p in params) * 0.0 for a per-parameter element read also removes a real bf16 hazard: a full-weight sum can overflow to inf, and inf * 0.0 is nan.
  • No dangling locals in the five refactored methods; no injected instructions anywhere in the diff.

Residual risk

Low. Every extraction is backed by an equality test, a hand-worked example, or the 23-case bitwise A/B, and the single deviation from the PR's own rule is gone.

One gap worth stating rather than blocking on: forward()'s early returns still read draft parameters, and the reported FSDP2 run exercised the in-loss zero branch, not that one. With the draft as its own fully_shard unit, a rank reaching n_blocks == 0 or not block_keep_mask.any() touches parameters FSDP2 has resharded — the same hazard the in-loss branch now avoids, and p.reshape(-1)[0] on a Shard(0) DTensor is not obviously cheaper than the p.sum() it replaces. This is pre-existing on main (DFlash already read parameters there) and cannot be fixed the same way, since no draft output exists yet at that point, so it is not a finding against this PR — but it is the one place where the refactor extends an untested pattern from one variant to three.

I did not execute tests (not permitted in this environment); all equivalence claims above come from reading the code.

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude review passed — no blocking issues found. LGTM

@ChenhanYu ChenhanYu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pensieve nudge review using chenhany's profile (revision 7).

Nudge: the prior DSpark blocker is resolved, but repository-wide compatibility and reuse checks remain unverified.

Needs action:

  • Confirm all _teacher_logits callers use label_positions, all configuration construction paths supply the directly accessed fields, and the extracted helpers do not duplicate existing repository utilities.

No action needed:

  • ✔️ Resolved: DSpark's zero loss now traverses both outputs, with gradient assertions. Reply 4390499509 explains why the requested distributed test would exercise an unsupported layout.
  • Existing teacher-logit test edits are justified by the label-position contract and preserve projection and candidate-gather assertions.
  • No new concrete defect was established. Review used the bounded diff and supplied history; repository navigation and test execution were unavailable, and omitted history could not be checked.

Review context was incomplete.

Review run: efebc52733794ff28f12d9840c30fce1

h-guo18 and others added 2 commits October 28, 2026 08:19
…hes; refuse an empty one

_draft_zero_loss's summary line promised a gradient for every trainable draft parameter.
Only the parameter route delivers that: the `through` route reaches just the parameters
behind the outputs it is handed. The docstring now says so, so a variant that adds a head
knows to pass that head's output too.

A `through` whose entries are all None used to return the int 0. It now raises. Falling back
to the parameter reads there would be wrong: once the draft has run, that is the route that
puts FSDP2's collectives out of step. No caller passes one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
…me a missing target_ce

decay_factor and dpace_alpha are adjacent floats that every caller passed positionally, and
swapped they still give a plausible weight curve. They are keyword-only now, as
label_positions is for the teacher.

D-PACE called target_ce without checking it. A caller that selects D-PACE without one now
gets a ValueError naming it, instead of "'NoneType' object is not callable". No behavior
change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
@h-guo18

h-guo18 commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

On the remaining check in the Pensieve review:

  • _teacher_logits callers. Every in-repo call passes label_positions=: hf_dflash.py, hf_dspark.py, hf_lilicorr.py and the teacher tests. HFExternalDraftModel._teacher_logits is a separate method with its own signature. Because the parameter is keyword-only, an out-of-tree caller that still passes logit rows positionally fails with TypeError instead of reading one row early.
  • Config fields. No getattr(config, "dflash_…", <default>) read is left. modify() is reached only from convert_to_dflash_model, which receives a DFlashConfig, and the recipe schema defaults to DFlashConfig(). Every field read directly therefore has its declared default.
  • Duplicated utilities. The only near match is HFExternalDraftModel._top1_accuracy in hf_external.py. It uses a different denominator (clamp_min(1) vs + 1e-6) and mask test (bool() vs > 0.5), so sharing it would change the DFlash family's logged accuracy. They stay separate.

@meenchen meenchen left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot review (gpt-6.1-sol) — DM the bot to share feedback.

Nudge: prior code concerns are resolved, but the PR description contradicts the implementation and needs the required reuse rationale.

Needs action:

  • Correct the PR’s generated summary: offline DSpark generation still raises, configuration fields retain schema defaults, and multimodal forwarding is not introduced by this diff.
  • Move the reuse rationale into the PR body: explain why External’s _top1_accuracy cannot preserve these metrics, and why extending HFDFlashModel and existing registries is sufficient without another policy/config abstraction.

No action needed:

  • ✔️ Resolved: DSpark’s output-connected zero loss, caller/config/reuse checks, and helper/test refinements.
  • Existing test edits are justified: they preserve teacher projection/gather coverage and strengthen zero-loss gradient coverage.

@h-guo18
h-guo18 requested review from a team, ChenhanYu and meenchen October 28, 2026 08:59

@ChenhanYu ChenhanYu left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pensieve nudge review using chenhany's profile (revision 7).

Nudge: prior concerns are addressed, but repository-wide verification and required current-head CI status remain unavailable.

Needs action:

  • Confirm required CI passes at a5d6a6b and obtain maintainer sign-off on integration checks unavailable in this snapshot.

No action needed:

  • ✔️ Resolved: DSpark gradient routing, helper guards and signatures, and the description/reuse concerns in review 5679986471. Reply 6464158010 specifically addresses caller, configuration, and utility compatibility.
  • Existing test edits preserve teacher projection/gather assertions and strengthen zero-loss gradient coverage.
  • No new concrete defect found. Repository navigation and test execution were unavailable; duplicate checking used supplied history, which may omit older discussion.

Review context was incomplete.

Review run: d6d5f7818d804816ac6dfdb9e2075b57

@ChenhanYu
ChenhanYu self-requested a review October 29, 2026 17:36
… as they were

Reverts 025e18a. Its two rows quote the offline AR-validation error, and HF AR
validation is being deprecated in favour of serving-engine evaluation, so the speculative
decoding skill is left for that change. The rows quote the per-variant messages again;
both still share "cannot run AR validation / pseudo_speculative_generate" with the
family-wide message, and their cause and fix still hold.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: h-guo18 <67671475+h-guo18@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants